Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: refactor to reuse validators #38608

Closed
wants to merge 2 commits into from
Closed

lib: refactor to reuse validators #38608

wants to merge 2 commits into from

Conversation

pd4d10
Copy link
Contributor

@pd4d10 pd4d10 commented May 9, 2021

No description provided.

@github-actions github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 9, 2021
@pd4d10 pd4d10 mentioned this pull request May 9, 2021
Open
Lxxyx
Lxxyx reviewed May 9, 2021
View reviewed changes
lib/_tls_wrap.js Outdated Show resolved Hide resolved
lib/internal/perf/timerify.js Show resolved Hide resolved
lib/internal/process/per_thread.js Outdated Show resolved Hide resolved
lib/util.js Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
lib/wasi.js Outdated Show resolved Hide resolved
lib/zlib.js Outdated
@@ -71,6 +71,7 @@ const {
const { owner_symbol } = require('internal/async_hooks').symbols;
const {
validateFunction,
validateNumber
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: trailing comma

Copy link
Contributor Author

@pd4d10 pd4d10 May 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. But the lint seems to pass. Shall we add this to the lint rule?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. But the lint seems passed. Shall we add this to the lint rule?

It would be nice if the lint rule could be added

@RaisinTen
Copy link
Contributor

Why is this happening? 👀

make[2]: Leaving directory '/home/runner/work/node/node/benchmark/napi/type-tag-check/build'
npm ERR! normalizeEncoding is not a function
make[1]: *** [Makefile:689: tools/doc/node_modules] Error 1

@pd4d10 pd4d10 requested a review from Lxxyx May 16, 2021 11:24
aduh95
aduh95 reviewed May 16, 2021
View reviewed changes
lib/events.js Outdated Show resolved Hide resolved
lib/_tls_wrap.js Outdated Show resolved Hide resolved
lib/_http_agent.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38129/

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

It seems that we should leave lib/util.js as is since lib/internal/validators.js depends on it, which is the cause of test failure.

Updated

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2021

It seems that we should leave lib/util.js as is since lib/internal/validators.js depends on it, which is the cause of test failure.

Most likely the error comes from lib/internal/util.js, not lib/utils.js: adding the require call there is creating a circular dependency hence the not defined error.

@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

It seems that we should leave lib/util.js as is since lib/internal/validators.js depends on it, which is the cause of test failure.

Most likely the error comes from lib/internal/util.js, not lib/utils.js: adding the require call there is creating a circular dependency hence the not defined error.

Yeah, you are right. I misread the file name.

Updated

@aduh95
Copy link
Contributor

aduh95 commented May 16, 2021

FYI force pushing makes the review process a bit harder, if you could prefer fixup commits instead that'd be nice. FYI you can update your local branch using git pull [email protected]:pd4d10/node.git patch-lib-validator.
Could you please reaply 1648190, it seems it was lost during the force-push.

@pd4d10 @aduh95
Apply suggestions from code review
9b29dcc 
Co-authored-by: Antoine du Hamel <[email protected]>
@pd4d10
Copy link
Contributor Author

pd4d10 commented May 16, 2021

FYI force pushing makes the review process a bit harder, if you could prefer fixup commits instead that'd be nice. FYI you can update your local branch using git pull [email protected]:pd4d10/node.git patch-lib-validator.
Could you please reaply 1648190, it seems it was lost during the force-push.

OK, get it. Updated

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38133/

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 16, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 16, 2021 •
edited by jasnell
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/38140/ 💚

pd4d10 added a commit to pd4d10/node that referenced this pull request May 18, 2021
@pd4d10
tools: update ESLint config comma-dangle to always-multiline
794fae8 
The advantage of doing this is that `eslint --fix` can automatically add
trailing commas, which avoids wasting time on manual formatting

Refs: nodejs#38701
Refs: nodejs#38608 (review)
@pd4d10 pd4d10 mentioned this pull request May 18, 2021
Closed
jasnell pushed a commit that referenced this pull request May 19, 2021
@pd4d10 @jasnell
lib: refactor to reuse validators
5d7b6c2 
PR-URL: #38608
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented May 19, 2021

Landed in 5d7b6c2

@jasnell jasnell closed this May 19, 2021
@pd4d10 pd4d10 deleted the patch-lib-validator branch May 19, 2021 16:26
danielleadams pushed a commit that referenced this pull request May 31, 2021
@pd4d10 @danielleadams
lib: refactor to reuse validators
2268d1c 
PR-URL: #38608
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zijian Liu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request May 31, 2021
Merged
@richardlau
Copy link
Member

This appears to depend on #37045 so adding a matching backport label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@Lxxyx Lxxyx Lxxyx approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@pd4d10 @RaisinTen @nodejs-github-bot @aduh95 @jasnell @richardlau @Trott @Lxxyx